Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move teamd warm reboot code to service script #5163

Merged
merged 19 commits into from
Nov 13, 2020

Conversation

heidinet2007
Copy link
Contributor

Move teamd warm reboot code to teamd service at /usr/local/bin

@heidinet2007
Copy link
Contributor Author

please re-test

@heidinet2007
Copy link
Contributor Author

please retest

files/scripts/teamd.sh Outdated Show resolved Hide resolved
1) Remove space in variable assignement
2) Apply multi-asic support
files/scripts/teamd.sh Outdated Show resolved Hide resolved
files/scripts/teamd.sh Outdated Show resolved Hide resolved
1) Remove "systemctl stop ${SERVICE}$DEV" for fast-reboot
2) Call /user/bin/teamd.sh to stop teamd for all cases
yxieca
yxieca previously approved these changes Oct 27, 2020
Copy link
Contributor

@vaibhavhd vaibhavhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments added. It would be also better if we have output logs for start,stop,wait.

files/scripts/teamd.sh Outdated Show resolved Hide resolved
files/scripts/teamd.sh Outdated Show resolved Hide resolved
files/scripts/teamd.sh Outdated Show resolved Hide resolved
start() {
debug "Starting ${SERVICE}$DEV service..."

check_warm_boot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also wait for database initialization before accessing it? I see that some of the service scripts use it, and some are missing it already.

https://github.com/Azure/sonic-buildimage/blob/master/files/scripts/swss.sh#L60
https://github.com/Azure/sonic-buildimage/blob/master/files/scripts/swss.sh#L137

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this wait_for_database_service() is called from swss and syncd. Teamd already has dependancy on swss.

@yxieca
Copy link
Contributor

yxieca commented Oct 28, 2020

retest broadcom please

@yxieca
Copy link
Contributor

yxieca commented Oct 28, 2020

retest vsimage please

1 similar comment
@yxieca
Copy link
Contributor

yxieca commented Oct 29, 2020

retest vsimage please

@yxieca
Copy link
Contributor

yxieca commented Oct 30, 2020

retest vsimage please

yxieca
yxieca previously approved these changes Oct 31, 2020
vaibhavhd
vaibhavhd previously approved these changes Oct 31, 2020
Copy link
Contributor

@vaibhavhd vaibhavhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Heidi.

@vaibhavhd
Copy link
Contributor

Retest vsimage please

@vaibhavhd
Copy link
Contributor

Retest mellanox please

@vaibhavhd
Copy link
Contributor

Retest vsimage please

3 similar comments
@vaibhavhd
Copy link
Contributor

Retest vsimage please

@vaibhavhd
Copy link
Contributor

Retest vsimage please

@vaibhavhd
Copy link
Contributor

Retest vsimage please

@yxieca
Copy link
Contributor

yxieca commented Nov 9, 2020

retest vsimage please

@vaibhavhd
Copy link
Contributor

The vsimage tests are failing due to incorrect permissions set on a new teamd script being added by this PR. Below is the fix:

$ git diff
diff --git a/files/scripts/teamd.sh b/files/scripts/teamd.sh
old mode 100644
new mode 100755

This would prevent the below error seen in syslog:

Nov  9 18:45:02.563249 sonic INFO systemd[1]: Starting TEAMD container...
Nov  9 18:45:02.564008 sonic ERR systemd[1867]: teamd.service: Failed to execute command: Permission denied
Nov  9 18:45:02.564329 sonic ERR systemd[1867]: teamd.service: Failed at step EXEC spawning /usr/local/bin/teamd.sh: Permission denied

@heidinet2007 I am unable to make this change on the fork. Can you make this change, or give me permission to do so?

Signed-off-by: Ying Xie <ying.xie@microsoft.com>
@yxieca yxieca dismissed stale reviews from vaibhavhd and themself via 8a4e634 November 13, 2020 01:54
@yxieca
Copy link
Contributor

yxieca commented Nov 13, 2020

The vsimage tests are failing due to incorrect permissions set on a new teamd script being added by this PR. Below is the fix:

$ git diff
diff --git a/files/scripts/teamd.sh b/files/scripts/teamd.sh
old mode 100644
new mode 100755

This would prevent the below error seen in syslog:

Nov  9 18:45:02.563249 sonic INFO systemd[1]: Starting TEAMD container...
Nov  9 18:45:02.564008 sonic ERR systemd[1867]: teamd.service: Failed to execute command: Permission denied
Nov  9 18:45:02.564329 sonic ERR systemd[1867]: teamd.service: Failed at step EXEC spawning /usr/local/bin/teamd.sh: Permission denied

@heidinet2007 I am unable to make this change on the fork. Can you make this change, or give me permission to do so?

@heidinet2007 I've pushed a change to your branch to change file permission.

@yxieca
Copy link
Contributor

yxieca commented Nov 13, 2020

retest vsimage please

Copy link
Contributor

@vaibhavhd vaibhavhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@vaibhavhd vaibhavhd merged commit 7c17c58 into sonic-net:master Nov 13, 2020
vaibhavhd added a commit to sonic-net/sonic-utilities that referenced this pull request Dec 1, 2020
A new teamd service script has been added in sonic-buildimage as part of the PR - sonic-net/sonic-buildimage#5163
With this change, fast-reboot script will now use the new teamd script to handle killing teamd docker in both fast-boot and warm-boot mode.

Replaced the existing teamd docker handling in fast-reboot script with the systemctl stop teamd.
Tested the locally modified script in a DUT for warm-reboot and fast-reboot.
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
Summary: Move teamd functions to a new service script

Motivation: To segregate teamd functions in one common place. fast-reboot script calls teamd functions that should ideally be replaced by a simple call to a service script.
 
Changes: New teamd service script and path modification from /usr/bin/teamd.sh to /usr/local/bin/teamd.sh
fast-reboot script (in sonic-utilities) modification (to use new teamd.sh to stop teamd) should follow soon after this change.

Verification: VS image tests.

Signed-off-by: Vaibhav Hemant Dixit <vaibhav.dixit@microsoft.com>

Co-authored-by: heidi.ou@alibaba-inc.com <heidi.ou@alibaba-inc.com>
Co-authored-by: Ying Xie <ying.xie@microsoft.com>
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
A new teamd service script has been added in sonic-buildimage as part of the PR - sonic-net/sonic-buildimage#5163
With this change, fast-reboot script will now use the new teamd script to handle killing teamd docker in both fast-boot and warm-boot mode.

Replaced the existing teamd docker handling in fast-reboot script with the systemctl stop teamd.
Tested the locally modified script in a DUT for warm-reboot and fast-reboot.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants